Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Makefile: evaluate os/arch based on target name #4236

Merged
merged 3 commits into from
May 20, 2020
Merged

Makefile: evaluate os/arch based on target name #4236

merged 3 commits into from
May 20, 2020

Conversation

tstromberg
Copy link
Contributor

@tstromberg tstromberg commented May 20, 2020 •
edited
Loading

Previously, the Makefile was setting a correct GOOS and GOARCH environment variables, but for flag values, it was passing in the native Makefile variables, set to the local OS and architecture that was executing the Makefile.

For example, running make out/skaffold-linux-amd64 on a macOS machine output:

GOOS="linux" GOARCH="amd64" \
	docker build \
		--build-arg GOOS=darwin \
		--build-arg GOARCH=amd64 \
		--build-arg TAGS="release" \
		--build-arg LDFLAGS=" -X

In this PR, I change from setting an environment variable, to setting a local variable ("os" and "arch"), which is evaluated from the Makefile rule. This local variable is then passed in correctly as command-line arguments. The previous example now emits:

docker build \
		--build-arg GOOS=linux \
		--build-arg GOARCH=amd64 \
		--build-arg TAGS="osusergo netgo static_build release" \
		--build-arg LDFLAGS=" -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.version=v1.10.0-4-gfafbfc163-dirty -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.buildDate=2020-05-20T11:28:19Z -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.gitCommit=fafbfc1636db2cfa49bcd0eb1ba0bd02fa921bb3 -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.gitTreeState=dirty -s -w  -extldflags \"-static\"" \
		-f deploy/cross/Dockerfile \
		-t skaffold/cross \
		.

You'll notice that TAGS and LDFLAGS are now properly evaluated as well.

Fixes #4234

@tstromberg tstromberg changed the title WIP: Makefile: evaluate os/arch based on target name Makefile: evaluate os/arch based on target name May 20, 2020
tejal29
tejal29 reviewed May 20, 2020
View reviewed changes
Makefile
@@ -75,17 +75,21 @@ install: $(BUILD_DIR)/$(PROJECT)
cross: $(foreach platform, $(SUPPORTED_PLATFORMS), $(BUILD_DIR)/$(PROJECT)-$(platform))

$(BUILD_DIR)/$(PROJECT)-%: $(STATIK_FILES) $(GO_FILES) $(BUILD_DIR) deploy/cross/Dockerfile
GOOS="$(firstword $(subst -, ,$*))" GOARCH="$(lastword $(subst -, ,$(subst .exe,,$*)))" \
$(eval os = $(firstword $(subst -, ,$*)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its is frustrating, we cant use keys values pairs all though its supported on bash4.
https://stackoverflow.com/questions/6047648/bash4-associative-arrays-error-declare-a-invalid-option
It would have been much easier to read and less error prone.

this change looks good to me.

@codecov
Copy link

codecov bot commented May 20, 2020 •
edited
Loading

Codecov Report

Merging #4236 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted Files Coverage Δ
pkg/skaffold/util/cmd.go 47.61% <0.00%> (-4.77%) ⬇️
pkg/skaffold/deploy/kustomize.go 74.71% <0.00%> (-1.51%) ⬇️
pkg/skaffold/deploy/kubectl/cli.go 90.90% <0.00%> (-0.40%) ⬇️
...affold/kubernetes/portforward/kubectl_forwarder.go 68.29% <0.00%> (+2.43%) ⬆️

@tstromberg tstromberg merged commit cc67c04 into GoogleContainerTools:master May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tejal29 tejal29 tejal29 approved these changes

@dgageot dgageot dgageot approved these changes

@balopat balopat Awaiting requested review from balopat

@briandealwis briandealwis Awaiting requested review from briandealwis

@nkubala nkubala Awaiting requested review from nkubala

Assignees
No one assigned
Labels
cla: yes size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

v1.10.0 binaries released for macOS & Windows are actually ELF 64-bit binaries (Linux)
4 participants
@tstromberg @dgageot @tejal29 @googlebot